Conversation
Manual Deploy AvailableYou can trigger a manual deploy of this PR branch to testnet: Alternative: Comment
Comment updated automatically when the PR is synchronized. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR changes two log formatting sites in fetch_cloner from Display to Debug, extends two RemoteAccountProviderError variants to include a limit String, threads an optional fetch_start_slot: Option through try_get_multi (updating its signature and call sites), records retry termination reasons ("max retries N" or "max total time of X seconds") and attaches them to errors, and updates tests to match the new error payloads and signature. Assessment against linked issues
Out-of-scope changes
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@magicblock-chainlink/src/remote_account_provider/errors.rs`:
- Around line 65-69: The error strings for SlotsDidNotMatch and
MatchingSlotsNotSatisfyingMinContextSlot need a separator before the appended
"hit limit:" clause; update the #[error(...)] messages for the enum variants
SlotsDidNotMatch and MatchingSlotsNotSatisfyingMinContextSlot to insert a comma
(or other suitable punctuation) just before "hit limit:" so the rendered
messages read naturally (e.g., "... to track slots, hit limit: ..." and "... min
required context slot 43, hit limit: ...").
* master: fix: wait for valid blockhash (#992)
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
magicblock-chainlink/src/remote_account_provider/mod.rs (1)
646-681: 🧹 Nitpick | 🔵 TrivialConsider using
>=instead of==for the retry-limit check.Using
==works today becauseretriesincrements by exactly 1 each iteration, but>=is a more defensive guard against future refactors that might alter the increment pattern.Suggested change
- let hit_max_retry_limit = retries == config.max_retries; + let hit_max_retry_limit = retries >= config.max_retries;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@magicblock-chainlink/src/remote_account_provider/mod.rs` around lines 646 - 681, The retry-limit check should be defensive: change how hit_max_retry_limit is computed so it uses >= rather than ==; specifically update the binding for hit_max_retry_limit (currently let hit_max_retry_limit = retries == config.max_retries) to use retries >= config.max_retries so the branch handling (the if hit_max_retry_limit || start.elapsed() > MAX_TOTAL_TIME { ... } block and subsequent error construction in RemoteAccountProviderError::SlotsDidNotMatch and RemoteAccountProviderError::MatchingSlotsNotSatisfyingMinContextSlot) still executes when retries has surpassed config.max_retries due to any future increment changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@magicblock-chainlink/src/remote_account_provider/mod.rs`:
- Around line 646-681: The retry-limit check should be defensive: change how
hit_max_retry_limit is computed so it uses >= rather than ==; specifically
update the binding for hit_max_retry_limit (currently let hit_max_retry_limit =
retries == config.max_retries) to use retries >= config.max_retries so the
branch handling (the if hit_max_retry_limit || start.elapsed() > MAX_TOTAL_TIME
{ ... } block and subsequent error construction in
RemoteAccountProviderError::SlotsDidNotMatch and
RemoteAccountProviderError::MatchingSlotsNotSatisfyingMinContextSlot) still
executes when retries has surpassed config.max_retries due to any future
increment changes.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
magicblock-chainlink/src/chainlink/fetch_cloner/mod.rsmagicblock-chainlink/src/remote_account_provider/mod.rs
thlorenz
changed the title
2 participants
Summary
Improve error messages for unresolved delegations and fetch failures by providing better insight into what caused the timeout or retry limit to be hit.
CLOSES: #988
Details
Reusing ChainSlot
Error Message Improvements
Enhanced error diagnostics in the remote account provider to distinguish between two timeout failure modes:
This allows operators to quickly understand which limit was hit and adjust configuration accordingly.
fetch_cloner
Changed error logging format from Display (
%) to Debug (?) for better error introspection when delegation record fetch fails.remote_account_provider
Updated error variants to include the specific limit that was hit:
SlotsDidNotMatch: Added limit string parameterMatchingSlotsNotSatisfyingMinContextSlot: Added limit string parameterThe timeout/retry logic now checks both conditions simultaneously and includes the appropriate limit description in error messages.
Summary by CodeRabbit